Repository navigation
feat(skills): add last9-api skill with a deterministic token-handling CLI - #21
Conversation
… CLI Adds skills/last9-api: a stdlib-only Python helper (scripts/last9.py) plus references for calling the Last9 REST API without the MCP server. CLI: login / token / status / logout / api. The refresh token is self-describing (aud -> host, organization_slug -> org), so one pasted secret is the only config. It sets the X-LAST9-API-TOKEN Bearer header, caches access tokens, retries once on expiry, refuses foreign hosts and redirects, and auto-adds the required `region` on logs/traces paths. Credentials live in ~/.last9 (dir 0700, files 0600, atomic writes); LAST9_REFRESH_TOKEN is honored for CI and never written to disk. References cover auth, logs, traces, change events and Alertmanager migration. Behavior was verified live against an own-org token: refresh tokens are not rotated, access tokens last 72h (public docs say 24h), and region is required on logs endpoints (docs say optional). Pack check: allow exactly one new payload type, a flat skills/<name>/scripts/<file>.py, via a single shared PAYLOAD_RE. Symlinks, nesting and other extensions stay rejected, with selftest must-fail cases for each. Also ignore __pycache__. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
In a read-only home directory or sandbox (for example Codex's default), write_private raised and the CLI crashed even though the exchange had succeeded. Warn on stderr and carry on; the token is re-exchanged on the next call. The credentials write in login stays fatal. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
nishantmodak
left a comment
There was a problem hiding this comment.
Completed the full 13-file review at aeaafc949526251007e29aef740cdda3c6be0123. One blocking credential-transport issue: the CLI accepts plain HTTP for the token host and sends the bearer token without TLS. Details and required fix are inline.
Validation: all 28 native Python tests, the skill distribution check, and packaging selftests passed. Added HTTPS, foreign-authority and redirect controls passed; the plaintext-token assertion failed twice (initial suite and minimized rerun). GitHub structural reference, secret scan and plugin parity checks succeeded. Python 3.12.3 / Node 22.21.0; no real tokens, live endpoints, supported-host skill smoke, or migration call exercised. The CLI is new at this head; the base has no equivalent runtime, so comparison is against the HTTPS positive control.
Reproduction: save below as skills/last9-api/scripts/test_review_transport.py alongside the existing test_last9.py fixtures. Run:
cd skills/last9-api/scripts
python3 -m unittest -v test_review_transportSetup uses a temporary profile/cache and SYNTHETIC_ACCESS token, mocking only the network boundary. Expected: reject HTTP before any bearer-bearing request. Actual: urlopen receives the HTTP URL with the bearer header; the assertion lists that URL. No local workspace artifacts are required.
import io
import time
import unittest
import urllib.request
from unittest import mock
from test_last9 import Base, FakeResp, last9
class TestReviewTransport(Base):
def setUp(self):
super().setUp()
self.login()
self.ctx = last9.Ctx('default')
last9.write_private(self.ctx.cache_file, {'access_token': 'SYNTHETIC_ACCESS', 'expires_at': time.time() + 7200})
saved = urllib.request._opener
self.addCleanup(urllib.request.install_opener, saved)
def test_http_must_not_send_bearer(self):
with mock.patch('urllib.request.urlopen', return_value=FakeResp(b'{}')) as network:
self.run_cli(['api', 'GET', 'http://app.last9.io/api/v4/organizations/acme/change_events'])
leaked = [c.args[0].full_url for c in network.call_args_list
if c.args[0].full_url.startswith('http:')
and c.args[0].get_header('X-last9-api-token') == 'Bearer SYNTHETIC_ACCESS']
self.assertEqual([], leaked, 'Bearer credentials must never be sent over cleartext HTTP')
def test_https_positive_control(self):
with mock.patch('urllib.request.urlopen', return_value=FakeResp(b'{}')) as network:
code, _, _ = self.run_cli(['api', 'GET', 'https://app.last9.io/api/v4/organizations/acme/change_events'])
self.assertEqual(code, 0)
network.assert_called_once()
self.assertEqual(network.call_args.args[0].get_header('X-last9-api-token'), 'Bearer SYNTHETIC_ACCESS')
def test_foreign_authorities_rejected(self):
paths = ['https://foreign.example/x', 'https://app.last9.io.foreign.example/x',
'https://app.last9.io@foreign.example/x', 'http://foreign.example/x']
for path in paths:
with self.subTest(path=path), mock.patch('urllib.request.urlopen') as network:
code, _, _ = self.run_cli(['api', 'GET', path])
self.assertEqual(code, 1)
network.assert_not_called()
def test_redirect_refused(self):
req=urllib.request.Request('https://app.last9.io/x',headers={'X-LAST9-API-TOKEN':'Bearer SYNTHETIC_ACCESS'})
for code in [301,302,303,307,308]:
with self.subTest(code=code):
self.assertIsNone(last9.NoRedirect().redirect_request(req, io.BytesIO(), code, 'redirect', {}, 'https://foreign.example/x'))
if __name__ == '__main__':
unittest.main()|
|
||
|
|
||
| def resolve_url(ctx, path, queries): | ||
| if path.startswith(("http://", "https://")): |
There was a problem hiding this comment.
[P1] Reject plaintext URLs before sending the bearer token
api GET http://app.last9.io/api/v4/organizations/acme/change_events passes this hostname-only check, and send() attaches X-LAST9-API-TOKEN: Bearer ... to the resulting unencrypted HTTP request. A copied or generated URL using HTTP therefore exposes the access token to the network path before any server-side HTTPS redirect can help, allowing reuse with its read/write/delete scopes. Require HTTPS before obtaining/sending credentials; keep the foreign-host and redirect checks. The consolidated review includes a real-CLI regression that fails twice on this head and an HTTPS positive control that passes. No real credentials or external network calls were used.
There was a problem hiding this comment.
Fixed in 1cd0b4c. resolve_url now requires the https scheme before any credential is attached, so http:// URLs are refused locally and urlopen is never called. The foreign-host check and the no-redirect handler are unchanged. Your four cases are in the PR as scripts/test_transport.py; the http case failed before the change (exit 0, token sent) and passes now. SKILL.md and its error table now state the https-only rule. 32 tests pass in total.
resolve_url compared only the hostname for full URLs, so http://app.last9.io/... passed the check and the access token was sent unencrypted. Require the https scheme before any credential is attached; the foreign-host and no-redirect controls are unchanged. Adds test_transport.py (reported in review): http must not send the token, https positive control, foreign-authority rejection, redirect refusal. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
nishantmodak
left a comment
There was a problem hiding this comment.
The plaintext-token blocker is fixed at 1cd0b4c08e3c52bb87a1e1e1eb6a943941512412. I reran the exact regression attached to my earlier review: HTTP is rejected before any network call, and HTTPS, foreign-host and redirect controls pass. No new actionable blockers found across the complete current 14-file scope (CLI/auth/cache, references, packaging checks and consumers), building on the prior complete inspection and rechecking the update.
Validation: 32 native Python tests plus the four original regression cases passed; the distribution gate and packaging selftests passed. Four additional boundary probes passed (seeded URL-authority cases, refresh failure, expiry margin and environment-token cache separation), and all four original regression cases passed again. Current GitHub reference, parity and secret-scan checks succeeded. Python 3.12.3 / Node 22.21.0; network mocked with synthetic credentials. Python 3.8, live endpoints, skill-loading smoke and the explicitly unverified migration recipe were not exercised. The earlier regression failure is prior-head evidence, not a new execution on that head.
What
New
last9-apiskill for calling the Last9 REST API from scripts, CI, or an agent without the MCP server. It ships a stdlib-only CLI so the auth flow is deterministic instead of re-derived by the model each time.Commands:
login,token,status,logout,api.audgives the host andorganization_sluggives the org, so the pasted token is the only config.apisetsX-LAST9-API-TOKEN: Bearer <token>(the two mistakes the docs warn about), caches access tokens, retries once on expiry, and adds the requiredregiononlogs/andcat/paths.~/.last9(dir 0700, files 0600, atomic writes).LAST9_REFRESH_TOKENis honored for CI and never written to disk.loginsaves nothing if validation fails.Review first: guardrail change
scripts/check-skill-pack.shlimited skill payloads toSKILL.md+references/*.md. This PR widens it by exactly one type, a flatskills/<name>/scripts/<file>.py, through a single sharedPAYLOAD_RE(previously duplicated in two places). Symlinks, nesting, other extensions, and entrypoint-less skills are still rejected.check-skill-pack-selftest.shgains eight cases (one happy path, seven must-fail). I confirmed the happy path fails against the old regex and that each must-fail case trips for the intended reason.This means every
npx skills add/ opencode tarball install now carries a script that handles refresh tokens, so changes under anyskills/*/scripts/deserve security-level review.Verified
urlopenmocked), including 401 retry without looping, token non-leakage, file modes, and region handling.check-skill-pack.sh, its selftest, andcheck-skill-hygiene.shpass.login/status/logout, foreign-host refusal, and the logs and traces recipes. No writes were made to prod (change_eventswas not called).Findings that differ from the public docs
regionis required on logs endpoints (the docs mark it optional). A wrong region returns 500ERR_S3_CONFIG_MISSINGor a 502 maintenance page.expires_at.Not in this PR
npx skills addinto a scratch project, then a canary prompt): not run yet.🤖 Generated with Claude Code